Add Async RedisCluster #2099

utkarshgupta137 · 2022-04-09T23:49:46Z

Pull Request check-list

Does $ tox pass with this change (including linting)?
Do the CI tests pass with this change (enable it first in your forked repo and wait for the github action build to finish)?
Is the new or changed code fully tested?
Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
Is there an example added to the examples folder (if applicable)?
Was the change added to CHANGES file?

Description of change

Add Async Redis Cluster Client

codecov-commenter · 2022-04-09T23:53:41Z

Codecov Report

Merging #2099 (9b1c983) into master (eca7075) will increase coverage by 0.17%.
The diff coverage is 94.86%.

@@            Coverage Diff             @@
##           master    #2099      +/-   ##
==========================================
+ Coverage   92.48%   92.66%   +0.17%     
==========================================
  Files         105      108       +3     
  Lines       25197    27174    +1977     
==========================================
+ Hits        23304    25180    +1876     
- Misses       1893     1994     +101

Impacted Files	Coverage Δ
redis/commands/parser.py	`88.23% <ø> (-0.18%)`	⬇️
tests/test_asyncio/test_connection.py	`97.36% <ø> (-0.14%)`	⬇️
tests/test_asyncio/test_retry.py	`100.00% <ø> (ø)`
whitelist.py	`0.00% <0.00%> (ø)`
redis/typing.py	`86.11% <57.14%> (-4.22%)`	⬇️
redis/asyncio/parser.py	`80.00% <80.00%> (ø)`
redis/asyncio/connection.py	`84.00% <84.90%> (-0.83%)`	⬇️
redis/asyncio/cluster.py	`89.08% <89.08%> (ø)`
tests/test_asyncio/conftest.py	`93.38% <92.85%> (-1.06%)`	⬇️
redis/commands/cluster.py	`94.68% <96.39%> (+1.10%)`	⬆️
... and 16 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eca7075...9b1c983. Read the comment docs.

chayim · 2022-04-10T07:24:10Z

@utkarshgupta137 This looks like a great PR in progress. Just checking if you meant for it to be draft.
CC: @dvora-h

utkarshgupta137 · 2022-04-10T08:14:55Z

@utkarshgupta137 This looks like a great PR in progress. Just checking if you meant for it to be draft. CC: @dvora-h

Yeah. I've not modified any documentation or added tests and I've only done basic testing.

utkarshgupta137 · 2022-04-10T23:15:01Z

@chayim @dvora-h @Andrew-Chen-Wan @Grokzen

I've got all the tests running, but there are some warnings that may need to be addressed.

I've divided the PR into 3 commits:

The first one simply duplicates all sync code in async locations. Also copies all mark.onlynoncluster from sync test_commands to asyncio test_commands
Second just changes all redis client calls in async cluster tests to async/await calls
Third one is where all the actual changes I've made in async cluster client & test modifications required to get it working.

chayim · 2022-04-11T06:03:13Z

@dvora-h Can you give @utkarshgupta137 a hand with this? How about integrating the documentation side - adding cluster examples, etc.

@utkarshgupta137 What else can we do to help? The last piece I see is the test_acl_log failure, at least from a starting position.

utkarshgupta137 · 2022-04-11T08:09:07Z

@dvora-h Can you give @utkarshgupta137 a hand with this? How about integrating the documentation side - adding cluster examples, etc.

@utkarshgupta137 What else can we do to help? The last piece I see is the test_acl_log failure, at least from a starting position.

That test is only failing on 3.6. If you see the cancelled 3.7+ tests, it hasn't failed in that case. Also ran normally on my amd64 mac py3.9.

The failure seems to be related to client closing. There are also warnings about the client not closing properly from other tests. Both of the problems are probably related. If anyone could verify the close() as well as the initialize() logic, that would be great.

utkarshgupta137 · 2022-04-11T14:30:05Z

Looks like the tests are failing on 3.6 due to related issue aio-libs-abandoned/aioredis-py#1273.

Andrew-Chen-Wang

just an initial read through. This looks great!

redis/cluster.py

redis/commands/parser.py

redis/asyncio/cluster.py

Andrew-Chen-Wang · 2022-04-14T12:01:50Z

gotcha. Cluster has an initialize async method. You can make a task that takes a ClusterNode as a param every time node manager creates a NodeCluster. Create a while loop with some small sleep time (because asyncio) that awaits for the task to be canceled. Cancel it when it needs to be removed. Once canceled (it’s an exception in the task itself), you can perform any disconnection on the ClusterNode within that loop. Something like: try: … except CanceledError (I think): pass finally: # disconnect

________________________________ From: Utkarsh Gupta ***@***.***> Sent: Thursday, April 14, 2022 7:56:21 AM To: redis/redis-py ***@***.***> Cc: Andrew Chen Wang ***@***.***>; Mention ***@***.***> Subject: Re: [redis/redis-py] Add Async RedisCluster (PR #2099) @utkarshgupta137 commented on this pull request.

________________________________ In redis/asyncio/cluster.py<#2099 (comment)>:

+ )

+ + def __eq__(self, obj): + return isinstance(obj, ClusterNode) and obj.name == self.name + + def __del__(self): + try: + if self.redis_connection is not None: + loop = asyncio.get_event_loop() + coro = self.redis_connection.close(True) + if loop.is_running(): + loop.create_task(coro) + else: + loop.run_until_complete(coro) + except Exception: + pass No, that's the problem. In case of MOVED errors or persistent connection errors, the nodes_manager would be reinitialized, which could lead to some Nodes being removed. — Reply to this email directly, view it on GitHub<#2099 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AOLG4VQ5BNWJ72PN5SXJ6PTVFABWLANCNFSM5S77AVMQ>. You are receiving this because you were mentioned.Message ID: ***@***.***>

utkarshgupta137 · 2022-04-15T01:03:27Z

@chayim @Andrew-Chen-Wang Most of the warnings have been resolved. The test_acl_log is still failing on 3.6, I'll check it tomorrow.

Andrew-Chen-Wang · 2022-04-15T01:06:17Z

@chayim when are we dropping Python 3.6? Or start marking it as unneeded for merging.

utkarshgupta137 · 2022-04-15T01:43:40Z

test_acl_log is fixed now. It was failing because I had an empty yield statement in async def.

chayim · 2022-04-17T09:17:50Z

@utkarshgupta137 Dropping python 3.6 support isn't a 4.2 thing, but could happen as we start to clean up the repo in 4.3+/5.x. There's a lot of 3.6 out there - even if I'd love to be 3.7+.

chayim · 2022-04-18T08:40:26Z

docs/conf.py

@@ -41,6 +42,10 @@
 autosectionlabel_prefix_document = True
 autosectionlabel_maxdepth = 2

+# AutodocTypehints settings.
+always_document_param_types = True


Awesome. Had no idea this was an option.

chayim

@utkarshgupta137 This is really, really close. I added tiny comments - that to be honest, I'm okay with eliminating. Also, it works nicely :).

I think we can get this in a 4.3.0 release, immediately - as this necessitates a minor version bump. WDYT?

utkarshgupta137 · 2022-04-27T11:33:20Z

@chayim Can you hold off on merging this? I think making it thread-safe is trivial.

chayim · 2022-04-27T12:22:21Z

Of course - but beware of scope creep.

…slots without lock

utkarshgupta137 · 2022-04-27T16:57:41Z

@chayim According to https://docs.python.org/3/library/asyncio-dev.html#asyncio-multithreading, asyncio is not meant to be used with threading anyway, so I've removed the threading related checks & added a test for asyncio.gather

Grokzen · 2022-04-27T22:03:43Z

@utkarshgupta137 I am no longer part of working with redis clients here or in my own repo any longer. I have handed the future of cluster related things to this repo so there won't be any need for me to review or be a part of any cluster related development :)

The major good part of this PR is that you get asyncio into here, the major pain going forward tho will be that it seems you have to clone and duplicate the normal code and asyncio code =( I am sure maintaining that is going to be a much bigger job after this get in.

Is the plan to require any future modifications and contributions to make both normal and asyncio code compatible with any change? or will it be more on a "lets hope it will happen" basis?

utkarshgupta137 · 2022-04-28T05:34:17Z

@Grokzen I will carry over all the changes/optimisations I've made to async to the sync client soon. For the long term, there is a discussion in #2119 regarding moving to sansio approach. It is IMO a much better approach for supporting both sync/async & I can port the cluster code to it if we go with it.

chayim · 2022-04-28T09:55:15Z

@chayim According to docs.python.org/3/library/asyncio-dev.html#asyncio-multithreading, asyncio is not meant to be used with threading anyway, so I've removed the threading related checks & added a test for asyncio.gather

I mean I use async and threading all the time - even if I shouldn't. But, never in prod. I think we're ready to merge this. @dvora-h ready for the new version when you are.

chayim · 2022-05-02T09:16:53Z

@utkarshgupta137 I just merged master in. We're very, very close!

Andrew-Chen-Wang · 2022-05-10T17:34:26Z

this looks great. thanks @utkarshgupta137 !!!!

karwinliu · 2022-05-18T18:49:11Z

Does this support pipeline by any chance?

utkarshgupta137 · 2022-05-18T18:57:48Z

Does this support pipeline by any chance?

Not at the moment. I just wanted to add basic support first and see if there are any issues. I will try to add pipeline support as well as a few other missing features like locks, pubsub, monitor etc.

karwinliu · 2022-05-18T19:02:03Z

Thanks @utkarshgupta137. What you have done on the async support is really helpful! Our project relies heavily on pipelines, so I wonder if there's any rough timeline you have in mind for these supports, or if there's any workaround for the time-being.

utkarshgupta137 · 2022-05-18T19:06:32Z

Thanks @utkarshgupta137. What you have done on the async support is really helpful! Our project relies heavily on pipelines, so I wonder if there's any rough timeline you have in mind for these supports, or if there's any workaround for the time-being.

I've been considering switching to pipelines as well. But between me writing the code, tests, review, & release, it may take 2-3 weeks. In the meantime, I think there are a couple of other packages with async cluster like aredis which may've support for it, but I'm not sure.

akx · 2022-09-19T11:39:48Z

Oof, there's a lot of copy-paste in this PR... My PyCharm's Locate Duplicates is just lighting up :(

utkarshgupta137 force-pushed the master branch from ec09005 to 51877f9 Compare April 9, 2022 23:53

utkarshgupta137 force-pushed the master branch 7 times, most recently from 23cc596 to ea79c95 Compare April 10, 2022 23:13

utkarshgupta137 marked this pull request as ready for review April 10, 2022 23:15

utkarshgupta137 force-pushed the master branch from ea79c95 to 716a690 Compare April 11, 2022 01:21

chayim mentioned this pull request Apr 14, 2022

Does it support asyncio for redis cluster? #2081

Closed

Andrew-Chen-Wang reviewed Apr 14, 2022

View reviewed changes

redis/cluster.py Outdated Show resolved Hide resolved

redis/commands/parser.py Show resolved Hide resolved

redis/asyncio/cluster.py Outdated Show resolved Hide resolved

utkarshgupta137 commented Apr 14, 2022

View reviewed changes

redis/asyncio/cluster.py Outdated Show resolved Hide resolved

utkarshgupta137 force-pushed the master branch from 2596ed6 to 977f221 Compare April 15, 2022 01:22

utkarshgupta137 force-pushed the master branch 3 times, most recently from 640e9ec to 7537178 Compare April 16, 2022 00:20

chayim reviewed Apr 18, 2022

View reviewed changes

chayim suggested changes Apr 27, 2022

View reviewed changes

async_cluster/parser: optimize _get_moveable_keys

0a48fff

utkarshgupta137 force-pushed the master branch 3 times, most recently from 82abb20 to 9f8e57a Compare April 27, 2022 10:58

utkarshgupta137 requested a review from chayim April 27, 2022 10:58

utkarshgupta137 added 3 commits April 27, 2022 22:19

async_cluster: inlined check_slots_coverage

9a4c741

async_cluster: update docstrings

4344145

async_cluster: add concurrent test & use read_response/_update_moved_…

22506e2

…slots without lock

utkarshgupta137 force-pushed the master branch from b9c0115 to 22506e2 Compare April 27, 2022 16:55

Merge remote-tracking branch 'redis/master'

a5f6e4b

chayim approved these changes Apr 28, 2022

View reviewed changes

utkarshgupta137 and others added 2 commits April 28, 2022 17:02

Merge remote-tracking branch 'original/master'

b3cd82b

Merge branch 'master' into master

9b1c983

chayim added the feature New feature label May 2, 2022

Merge branch 'master' into master

8e0b461

dvora-h merged commit 061d97a into redis:master May 8, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Async RedisCluster #2099

Add Async RedisCluster #2099

utkarshgupta137 commented Apr 9, 2022 •

edited

Loading

codecov-commenter commented Apr 9, 2022 •

edited

Loading

chayim commented Apr 10, 2022

utkarshgupta137 commented Apr 10, 2022

utkarshgupta137 commented Apr 10, 2022 •

edited

Loading

chayim commented Apr 11, 2022

utkarshgupta137 commented Apr 11, 2022 •

edited

Loading

utkarshgupta137 commented Apr 11, 2022 •

edited

Loading

Andrew-Chen-Wang left a comment

Andrew-Chen-Wang commented Apr 14, 2022 via email

utkarshgupta137 commented Apr 15, 2022

Andrew-Chen-Wang commented Apr 15, 2022

utkarshgupta137 commented Apr 15, 2022 •

edited

Loading

chayim commented Apr 17, 2022

chayim Apr 18, 2022

chayim left a comment

utkarshgupta137 commented Apr 27, 2022

chayim commented Apr 27, 2022

utkarshgupta137 commented Apr 27, 2022 •

edited

Loading

Grokzen commented Apr 27, 2022

utkarshgupta137 commented Apr 28, 2022

chayim commented Apr 28, 2022

chayim commented May 2, 2022

Andrew-Chen-Wang commented May 10, 2022 •

edited

Loading

karwinliu commented May 18, 2022

utkarshgupta137 commented May 18, 2022

karwinliu commented May 18, 2022 •

edited

Loading

utkarshgupta137 commented May 18, 2022

akx commented Sep 19, 2022 •

edited

Loading

Add Async RedisCluster #2099

Add Async RedisCluster #2099

Conversation

utkarshgupta137 commented Apr 9, 2022 • edited Loading

Pull Request check-list

Description of change

codecov-commenter commented Apr 9, 2022 • edited Loading

Codecov Report

chayim commented Apr 10, 2022

utkarshgupta137 commented Apr 10, 2022

utkarshgupta137 commented Apr 10, 2022 • edited Loading

chayim commented Apr 11, 2022

utkarshgupta137 commented Apr 11, 2022 • edited Loading

utkarshgupta137 commented Apr 11, 2022 • edited Loading

Andrew-Chen-Wang left a comment

Choose a reason for hiding this comment

Andrew-Chen-Wang commented Apr 14, 2022 via email

utkarshgupta137 commented Apr 15, 2022

Andrew-Chen-Wang commented Apr 15, 2022

utkarshgupta137 commented Apr 15, 2022 • edited Loading

chayim commented Apr 17, 2022

chayim Apr 18, 2022

Choose a reason for hiding this comment

chayim left a comment

Choose a reason for hiding this comment

utkarshgupta137 commented Apr 27, 2022

chayim commented Apr 27, 2022

utkarshgupta137 commented Apr 27, 2022 • edited Loading

Grokzen commented Apr 27, 2022

utkarshgupta137 commented Apr 28, 2022

chayim commented Apr 28, 2022

chayim commented May 2, 2022

Andrew-Chen-Wang commented May 10, 2022 • edited Loading

karwinliu commented May 18, 2022

utkarshgupta137 commented May 18, 2022

karwinliu commented May 18, 2022 • edited Loading

utkarshgupta137 commented May 18, 2022

akx commented Sep 19, 2022 • edited Loading

utkarshgupta137 commented Apr 9, 2022 •

edited

Loading

codecov-commenter commented Apr 9, 2022 •

edited

Loading

utkarshgupta137 commented Apr 10, 2022 •

edited

Loading

utkarshgupta137 commented Apr 11, 2022 •

edited

Loading

utkarshgupta137 commented Apr 11, 2022 •

edited

Loading

utkarshgupta137 commented Apr 15, 2022 •

edited

Loading

utkarshgupta137 commented Apr 27, 2022 •

edited

Loading

Andrew-Chen-Wang commented May 10, 2022 •

edited

Loading

karwinliu commented May 18, 2022 •

edited

Loading

akx commented Sep 19, 2022 •

edited

Loading